Skip to content

Permit to Inject GlpiAsset - #620

Draft
tsmr wants to merge 24 commits into
pluginsGLPI:mainfrom
InfotelGLPI:InjectGlpiAsset
Draft

Permit to Inject GlpiAsset#620
tsmr wants to merge 24 commits into
pluginsGLPI:mainfrom
InfotelGLPI:InjectGlpiAsset

Conversation

@tsmr

@tsmr tsmr commented May 7, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Checklist before requesting a review

Please delete options that are not relevant.

  • I have performed a self-review of my code.
  • I have added tests (when available) that prove my fix is effective or that my feature works.
  • I have updated the CHANGELOG with a short functional description of the fix or new feature.
  • This change requires a documentation update.

Description

Permit to Inject GlpiAsset (and add Injectable Capacity)
Warning this development don't add type & model injection of GlpiAsset

@Rom1-B
Rom1-B requested review from Rom1-B and stonebuzz May 7, 2026 13:55
Rom1-B
Rom1-B requested changes May 11, 2026
View reviewed changes
Comment thread inc/commoninjectionlib.class.php Outdated
@Herafia

Herafia commented Jul 21, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Hello @tsmr,
You can use these commands to resolve the CI issue:
../../vendor/bin/phpstan
../../vendor/bin/php-cs-fixer fix
../../vendor/bin/rector
php bin/console tools:licence_headers_check

Thank you!
Laura - Teclib

@Herafia
Herafia requested a review from Rom1-B July 21, 2026 13:03
Rom1-B
Rom1-B requested changes Jul 22, 2026
View reviewed changes
Comment thread src/Glpi/Asset/AssetInjection.php
@Herafia
Herafia requested a review from Rom1-B July 28, 2026 08:19
Rom1-B
Rom1-B requested changes Aug 3, 2026
View reviewed changes
Comment thread src/Glpi/Asset/AssetInjection.php Outdated
Comment thread src/Glpi/Asset/AssetInjection.php Outdated
Comment thread src/Glpi/Asset/AssetInjection.php Outdated
Comment thread src/Glpi/Asset/AssetInjection.php Outdated
tsmr and others added 5 commits August 17, 2026 11:54
@tsmr
Merge branch 'pluginsGLPI:main' into InjectGlpiAsset
a8181fe
@tsmr @Rom1-B
Update src/Glpi/Asset/AssetInjection.php
99bbe17 
Co-authored-by: Romain B. <8530352+Rom1-B@users.noreply.github.com>
@tsmr @Rom1-B
Update src/Glpi/Asset/AssetInjection.php
bf37ab3 
Co-authored-by: Romain B. <8530352+Rom1-B@users.noreply.github.com>
@tsmr @Rom1-B
Update src/Glpi/Asset/AssetInjection.php
b3ffc43 
Co-authored-by: Romain B. <8530352+Rom1-B@users.noreply.github.com>
@tsmr @Rom1-B
Update src/Glpi/Asset/AssetInjection.php
f4cb5f8 
Co-authored-by: Romain B. <8530352+Rom1-B@users.noreply.github.com>
stonebuzz
stonebuzz requested changes Aug 18, 2026
View reviewed changes
Comment thread setup.php
Comment thread src/Glpi/Asset/Capacity/IsInjectableCapacity.php Outdated
Comment thread setup.php
Comment thread src/Glpi/Asset/AssetInjection.php
Comment thread src/Glpi/Asset/AssetInjection.php Outdated
Comment thread src/Glpi/Asset/AssetInjection.php Outdated
Comment thread src/Glpi/Asset/Capacity/IsInjectableCapacity.php Outdated
Comment thread setup.php Outdated
tsmr and others added 6 commits August 19, 2026 12:02
@tsmr @stonebuzz
Update setup.php
d8c459c 
Co-authored-by: Stanislas <skita@teclib.com>
@tsmr @stonebuzz
Update setup.php
336c563 
Co-authored-by: Stanislas <skita@teclib.com>
@tsmr @stonebuzz
Update src/Glpi/Asset/AssetInjection.php
452f6ed 
Co-authored-by: Stanislas <skita@teclib.com>
@tsmr @stonebuzz
Update setup.php
d531a7f 
Co-authored-by: Stanislas <skita@teclib.com>
@tsmr
Clean files
8d5fc51
@tsmr
Fix install
f26f408
@Herafia
Herafia self-requested a review September 4, 2026 12:32
@Mirkk

Mirkk commented Sep 4, 2026

Copy link
Copy Markdown

Hi all! Thank you for this great PR! We tested it and it seems to work well:

  • import of basic data works
  • import of standard dropdowns works
  • import of OS, Port, IP (NetworkName) works

We're happy it works now, and of course don't pretend that it is be perfect, however we have some feedback, in order of importance:

  1. Type and Model Dropdowns not shown to be imported
  2. "Comment" is not shown to be imported (often very important in CSV imports)
  3. Custom Dropdowns are not shown to be imported
  4. (minor as it seems a generic problem in the last versions): with port unicity criteria set to "name" still an new IP (networkname) for the same name is added, not replaced)

Thank you!

RomainLvr
RomainLvr reviewed Sep 18, 2026
View reviewed changes

@RomainLvr RomainLvr left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re Mirkk's feedback (point 3, custom dropdowns): custom fields use search option ids >= 45000 (CustomFieldDefinition::getSearchOptionID()), but PluginDatainjectionCommonInjectionLib::addToSearchOptions() only flags ids < 1000 as injectable, so they're dropped before the blacklist logic even runs. Even after lifting that, every custom field shares the same linkfield (custom_fields, since values are stored in one JSON column), and addToSearchOptions() keeps only one option per linkfield (dedup added for issue #121) - so only one custom field could ever be importable at a time, and writing a resolved value back would need to target custom_<system_name> rather than custom_fields directly (see Asset::handleCustomFieldsUpdate()). That's bigger than a one-line fix, flagging it so it's tracked rather than silently missing.

Comment thread src/Glpi/Asset/AssetInjection.php
Comment on lines +154 to +155
unset($tab[4]);
unset($tab[40]);

@RomainLvr RomainLvr Sep 18, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing these two lines drops the Type (4) and Model (40) dropdown options entirely, so they can never be imported. They come from Asset::rawSearchOptions() and behave like any other dropdown option already supported here (e.g. Manufacturer, 23).

Suggested change
unset($tab[4]);
unset($tab[40]);

@tsmr tsmr Sep 18, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As say into the PR : Warning this development don't add type & model injection of GlpiAsset ? So you can continue the development if you want

Comment thread src/Glpi/Asset/AssetInjection.php
@tsmr @RomainLvr
Update src/Glpi/Asset/AssetInjection.php
71dbc6f 
Co-authored-by: Romain Lecouvreur <102067890+RomainLvr@users.noreply.github.com>
@RomainLvr
RomainLvr marked this pull request as draft September 22, 2026 14:18
@RomainLvr RomainLvr self-assigned this Sep 22, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@RomainLvr RomainLvr RomainLvr left review comments
@stonebuzz stonebuzz stonebuzz requested changes
@Rom1-B Rom1-B Rom1-B requested changes
@Herafia Herafia Awaiting requested review from Herafia
+1 more reviewer
@smurdza smurdza smurdza left review comments
Reviewers whose approvals may not affect merge requirements

Requested changes must be addressed to merge this pull request.

Assignees

@RomainLvr RomainLvr

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@tsmr @Herafia @Mirkk @stonebuzz @Rom1-B @smurdza @RomainLvr